Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Query: Adds support for non streaming ORDER BY #4362

Merged
merged 36 commits into from
Apr 1, 2024
Merged

Query: Adds support for non streaming ORDER BY #4362

merged 36 commits into from
Apr 1, 2024

Conversation

neildsh
Copy link
Contributor

@neildsh neildsh commented Mar 20, 2024 •
edited
Loading

Description

This change adds support for non streaming ORDER BY. Prior to this, the SDK assumed that the documents returned by the backend in response to ORDER BY queries were totally ordered across continuations. With the addition of non streaming ORDER BY to the backend, this is no longer true. The backend indicates this with a new field in the response body, _streaming that will be set to false. In this case, a new non streaming order by pipeline stage is created. This is a blocking pipeline stage similar to GROUP BY. This stage accumulates all backend responses before it yields any results. This stage is exercised only if _streaming is set to false in the backend response to an ORDER BY query.

This change also fixes a few bugs in the SDK code:

The ResponseLengthInBytes property of QueryPage was being set incorrectly in several pipeline stages and has been removed.

The IQueryPipelineStage interface had an awkward SetCancellationToken method that has been removed in favor of taking the CancellationToken as a parameter to the MoveNextAsync method on ITracingAsyncEnumerator instead.

The index utilization header was not being accumulated correctly in SkipEmptyPageQueryPipelineStage, and similar code in ClientAggregateQueryPipelineStage had an unnecessary allocation in a loop.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Closing issues

To automatically close an issue: closes #IssueNumber

@sboshra
Copy link
Contributor

sboshra commented Mar 20, 2024 •
edited by neildsh
Loading 

    private sealed class StaticQueryPageParameters

I'd avoid 'Static' here #Resolved

Refers to: Microsoft.Azure.Cosmos/src/Query/Core/Pipeline/CrossPartition/OrderBy/OrderByCrossPartitionQueryPipelineStage.cs:71 in dd5ae23. [](commit_id = dd5ae23475604edfca950939b757aa0ea313e4dd, deletion_comment = False)

@sboshra
Copy link
Contributor

sboshra commented Mar 20, 2024 •
edited by neildsh
Loading 

        public int MaxConcurrency { get; }

new lines #Resolved

Refers to: Microsoft.Azure.Cosmos/src/Query/Core/Pipeline/CrossPartition/OrderBy/OrderByCrossPartitionQueryPipelineStage.cs:50 in dd5ae23. [](commit_id = dd5ae23475604edfca950939b757aa0ea313e4dd, deletion_comment = False)

@sboshra
Copy link
Contributor

sboshra commented Mar 20, 2024 •
edited by neildsh
Loading 

    private OrderByCrossPartitionQueryPipelineStage(InitializationParameters init)

parameters or something else #Resolved

Refers to: Microsoft.Azure.Cosmos/src/Query/Core/Pipeline/CrossPartition/OrderBy/OrderByCrossPartitionQueryPipelineStage.cs:110 in dd5ae23. [](commit_id = dd5ae23475604edfca950939b757aa0ea313e4dd, deletion_comment = False)

@sboshra
Copy link
Contributor

sboshra commented Mar 20, 2024 •
edited by neildsh
Loading 

                return true;

this.bufferedPages.Count > 0 #Resolved

Refers to: Microsoft.Azure.Cosmos/src/Query/Core/Pipeline/CrossPartition/OrderBy/OrderByCrossPartitionQueryPipelineStage.cs:152 in dd5ae23. [](commit_id = dd5ae23475604edfca950939b757aa0ea313e4dd, deletion_comment = False)

@sboshra
Copy link
Contributor

sboshra commented Mar 20, 2024 •
edited by neildsh
Loading

Should this be added as a resource value? #ByDesign

Refers to: Microsoft.Azure.Cosmos/src/Query/Core/Pipeline/CrossPartition/OrderBy/OrderByCrossPartitionQueryPipelineStage.cs:1885 in eba9d5a. [](commit_id = eba9d5a, deletion_comment = False)

@sboshra
Copy link
Contributor

sboshra commented Mar 20, 2024 •
edited by neildsh
Loading 

        {

Consider only using the MultiLevel class #Pending

Refers to: Microsoft.Azure.Cosmos/src/Query/Core/Pipeline/CrossPartition/OrderBy/OrderByCrossPartitionQueryPipelineStage.cs:2031 in 4271b86. [](commit_id = 4271b8691ad67e7eb7605b19a5ebd85efad439fc, deletion_comment = False)

sboshra
sboshra previously approved these changes Mar 22, 2024
View reviewed changes
Copy link
Contributor

@sboshra sboshra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@neildsh neildsh force-pushed the users/ndeshpan/nonStreamingOrderBy branch from 4271b86 to d77567d Compare March 26, 2024 17:33
@neildsh
Copy link
Contributor Author

neildsh commented Mar 27, 2024

All the other pipeline stages also use inline constants. This code will be invoked in loop, where we may not want to pay the performance cost of doing a resource lookup. I will keep this as is for now.

In reply to: 2010479803

Refers to: Microsoft.Azure.Cosmos/src/Query/Core/Pipeline/CrossPartition/OrderBy/OrderByCrossPartitionQueryPipelineStage.cs:1885 in eba9d5a. [](commit_id = eba9d5a, deletion_comment = False)

@neildsh
Initial prep for adding a non streaming order by query pipline stage
2d90f0f
@neildsh
Change TracingAsyncEnumerator to be an adapter class between ITracing…
92a0625 
…AsyncEnumerator and IAsyncEnumerator
@neildsh
Move TracingAsyncEnumerator to product code
0eff840
@neildsh
Change ITracingAsyncEnumerator.MoveNext to take a CancellationToken, …
3ae0b62 
…thereby removing the awkward method SetCancellationToken from IQueryPipelineStage
@neildsh
draft implementation of non streaming order by pipeline stage
0ff0687
@neildsh
Fix bug that drops first record on an initialized page
b252123
@neildsh
Add a test class for non streaming order by unit tests
163f31f
@neildsh
remove unnecessary usings
83e1726
@neildsh
Add an emulator test for non streaming order by. Also lay groundwork …
bf2e7dd 
…for an alternative implementation for non streaming order by
@neildsh neildsh force-pushed the users/ndeshpan/nonStreamingOrderBy branch from dd5ae23 to 3065ed1 Compare March 28, 2024 07:12
adityasa
adityasa approved these changes Mar 29, 2024
View reviewed changes
Copy link
Contributor

@adityasa adityasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@neildsh
Copy link
Contributor Author

neildsh commented Mar 30, 2024

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@neildsh
Copy link
Contributor Author

neildsh commented Apr 1, 2024

/azp automerge

@azure-pipelines Azure Pipelines
Copy link

Command 'automerge' is not supported by Azure Pipelines.

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

@Maya-Painter Maya-Painter added QUERY auto-merge Enables automation to merge PRs labels Apr 1, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot merged commit 5788326 into master Apr 1, 2024
21 checks passed
@microsoft-github-policy-service microsoft-github-policy-service bot deleted the users/ndeshpan/nonStreamingOrderBy branch April 1, 2024 17:21
@simorenoh simorenoh mentioned this pull request May 3, 2024
Merged
5 tasks
This was referenced May 3, 2024
Merged
Merged
Merged
Merged
@aayush3011 aayush3011 mentioned this pull request May 10, 2024
Merged
6 tasks
@neildsh neildsh mentioned this pull request May 15, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sboshra sboshra sboshra approved these changes

@adityasa adityasa adityasa approved these changes

@khdang khdang khdang approved these changes

@kirankumarkolli kirankumarkolli Awaiting requested review from kirankumarkolli kirankumarkolli is a code owner

@ealsur ealsur Awaiting requested review from ealsur

@FabianMeiswinkel FabianMeiswinkel Awaiting requested review from FabianMeiswinkel FabianMeiswinkel is a code owner

@kirillg kirillg Awaiting requested review from kirillg kirillg is a code owner

Assignees
No one assigned
Labels
auto-merge Enables automation to merge PRs QUERY
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@neildsh @sboshra @khdang @adityasa @Maya-Painter